-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for pack to extend base images using daemon #1791
base: main
Are you sure you want to change the base?
Conversation
12db41c
to
28f6828
Compare
Signed-off-by: Darshan Kumar <[email protected]>
28f6828
to
ca07b65
Compare
Signed-off-by: Darshan Kumar <[email protected]>
951066a
to
abdf62c
Compare
Moving this one to milestone 0.31.0 |
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
e286d48
to
8710e46
Compare
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
Signed-off-by: Darshan Kumar <[email protected]>
d63c890
to
7cf7e08
Compare
@itsdarshankumar thank you for making all the updates to your branch. I tried to re-run the test mentioned in #1791 (comment), but I'm still getting the error: Before we proceed further, it might be prudent to check with @loewenstein @c0d1ngm0nk3y and others to see if there is still appetite for this change, given forthcoming changes to the extend phase outlined in buildpacks/spec#384. To summarize, with #1791 we'd use the daemon to extend builder images only (effectively chaining |
I will leave a thorough technical comment up to @pbusko @c0d1ngm0nk3y @modulo11 @phil9909 - but I am wondering how a |
@loewenstein it wouldn't make the lifecycle change harder - but, some things would need to be re-implemented in pack since we aren't relying on the lifecycle for build image extension. |
FWIW buildpacks/lifecycle#1276 is said lifecycle implementation ;) |
Signed-off-by: Darshan Kumar <[email protected]> Signed-off-by: Darshan Kumar <[email protected]>
b680823
to
abb040c
Compare
@itsdarshankumar thank you for your work here. Given the evolution of the "Dockerfiles" feature and the introduction of configurable context directories (buildpacks/spec#384), it may no longer by prudent to have a separate implementation of build image extension that uses the daemon, as this would require changes to be implemented twice (once in the lifecycle for run image extension and non-pack platforms, and once in pack for build image extension). Also having build image extension (uses daemon) and run image extension (uses lifecycle) behave differently in pack might be confusing, especially for newer contributors. However the work in this PR is still extremely valuable as it gives us an indication of the performance benefits we might see in relying on the daemon for image extension vs kaniko. I suggest we keep on eye on potential upcoming changes to the daemon (such as moby/moby#44369) that might run image extension with the daemon more performant. If build image extension and run image extension could both use the daemon, that would make things simpler in pack. That doesn't solve the need for changes to be implemented twice (lifecycle, pack) but we can still learn from this PR how to make that easier, if we decide the performance improvement is worth the effort. |
Summary
As pack already has access to a daemon, it can apply the dockerfiles directly, saving the extended build base image in the daemon which gives us great performance benefits. Thus it will not need to use the extender phase of lifecycle. Additionally it will drop the requirement that the image being extended must be published to a registry.
Output
Related
Resolves #1623